Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding disable/gray image props in ImageData when rescaled #1347

Merged
merged 1 commit into from
Jul 26, 2024

Conversation

ShahzaibIbrahim
Copy link
Contributor

@ShahzaibIbrahim ShahzaibIbrahim commented Jul 17, 2024

Disabled image do not work when rescaled. Writing a new method copyImageDataForDisabledOrGrayImages and calling it before init image in win32_getHandle.

HOW TO TEST

Put the following snippet into the snippet folder to test the effect:

/*******************************************************************************
 * Copyright (c) 2015, 2018 IBM Corporation and others.
 *
 * This program and the accompanying materials
 * are made available under the terms of the Eclipse Public License 2.0
 * which accompanies this distribution, and is available at
 * https://www.eclipse.org/legal/epl-2.0/
 *
 * SPDX-License-Identifier: EPL-2.0
 *
 * Contributors:
 *     IBM Corporation - initial API and implementation
 *******************************************************************************/
package org.eclipse.swt.snippets;

import org.eclipse.swt.*;
import org.eclipse.swt.graphics.*;
import org.eclipse.swt.layout.*;
import org.eclipse.swt.widgets.*;

public class Snippet382 {
	private static final String IMAGE_100 = "eclipse16.png";
	private static final String IMAGE_150 = "eclipse24.png";
	private static final String IMAGE_200 = "eclipse32.png";
	private static final String IMAGES_ROOT = "bin/org/eclipse/swt/snippets/";

	private static final String IMAGE_PATH_100 = IMAGES_ROOT + IMAGE_100;
	private static final String IMAGE_PATH_150 = IMAGES_ROOT + IMAGE_150;
	private static final String IMAGE_PATH_200 = IMAGES_ROOT + IMAGE_200;

	public static void main (String [] args) {
		final ImageFileNameProvider filenameProvider = zoom -> {
			switch (zoom) {
			case 100:
				return IMAGE_PATH_100;
			case 150:
				return IMAGE_PATH_150;
			case 200:
				return IMAGE_PATH_200;
			default:
				return null;
			}
		};
		final ImageDataProvider imageDataProvider = zoom -> {
			switch (zoom) {
			case 100:
				return new ImageData (IMAGE_PATH_100);
			case 150:
				return new ImageData (IMAGE_PATH_150);
			case 200:
				return new ImageData (IMAGE_PATH_200);
			default:
				return null;
			}
		};

		final Display display = new Display ();
		final Shell shell = new Shell (display);
		shell.setText("Snippet367");
		shell.setLayout (new GridLayout (3, false));
		Listener l = new Listener() {
			@Override
			public void handleEvent(Event e)  {
				if (e.type == SWT.Paint) {
					GC mainGC = e.gc;
					GCData gcData = mainGC.getGCData();
					final Image imageWithFileNameProvider = new Image (display, filenameProvider);
					final Image disabledImageWithFileNameProvider = new Image (display,imageWithFileNameProvider, SWT.IMAGE_DISABLE);
					final Image greyImageWithFileNameProvider = new Image (display,imageWithFileNameProvider, SWT.IMAGE_GRAY);

					final Image imageWithDataProvider = new Image (display, imageDataProvider);
					final Image disabledImageWithDataProvider = new Image (display,imageWithDataProvider, SWT.IMAGE_DISABLE);
					final Image greyImageWithDataProvider = new Image (display,imageWithDataProvider, SWT.IMAGE_GRAY);

					final Image imageWithData = new Image (display, IMAGE_PATH_100);
					final Image disabledImageWithData = new Image (display,imageWithData, SWT.IMAGE_DISABLE);
					final Image greyImageWithData = new Image (display,imageWithData, SWT.IMAGE_GRAY);

					try {
						drawImages(mainGC, gcData, "Normal",40, imageWithFileNameProvider);
						drawImages(mainGC, gcData, "Disabled",80, disabledImageWithFileNameProvider);
						drawImages(mainGC, gcData, "Greyed",120, greyImageWithFileNameProvider);

						drawImages(mainGC, gcData, "Normal",160, imageWithDataProvider);
						drawImages(mainGC, gcData, "Disabled",200, disabledImageWithDataProvider);
						drawImages(mainGC, gcData, "Greyed",240, greyImageWithDataProvider);

						drawImages(mainGC, gcData, "Normal",280, imageWithDataProvider);
						drawImages(mainGC, gcData, "Disabled",320, disabledImageWithData);
						drawImages(mainGC, gcData, "Greyed",360, greyImageWithData);
					} finally {
						mainGC.dispose ();
					}
				}
			}

			private void drawImages(GC mainGC, GCData gcData, String text, int y, final Image imageWithFileNameProvider) {
				gcData.nativeZoom = 100;
				mainGC.drawText(text, 0, y);
				mainGC.drawImage(imageWithFileNameProvider, 50, y);
				gcData.nativeZoom = 150;
				mainGC.drawImage(imageWithFileNameProvider, 100, y);
				gcData.nativeZoom = 200;
				mainGC.drawImage(imageWithFileNameProvider, 150, y/2);
			}
		};
		shell.addListener(SWT.Paint, l);




		shell.setSize(600, 600);
		shell.open ();
		while (!shell.isDisposed ()) {
			if (!display.readAndDispatch ()) display.sleep ();
		}
		display.dispose ();
	}

}

image

Copy link
Contributor

github-actions bot commented Jul 17, 2024

Test Results

   478 files  ±0     478 suites  ±0   7m 55s ⏱️ +22s
 4 147 tests ±0   4 139 ✅ ±0   8 💤 ±0  0 ❌ ±0 
16 354 runs  ±0  16 262 ✅ ±0  92 💤 ±0  0 ❌ ±0 

Results for commit fefe52f. ± Comparison against base commit c3f7474.

♻️ This comment has been updated with latest results.

@ShahzaibIbrahim ShahzaibIbrahim marked this pull request as ready for review July 17, 2024 09:57
@ShahzaibIbrahim ShahzaibIbrahim force-pushed the master-94 branch 3 times, most recently from 5c548f2 to 49fb9dc Compare July 24, 2024 08:31
Copy link
Contributor

@fedejeanne fedejeanne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested it and it has the expected behavior ✔️

There's only one small typo (missing "e") and I would like to have the snippet also contributed. You need to set the name of the dialog properly though: shell.setText("Snippet382");

Copy link
Contributor

@fedejeanne fedejeanne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I corrected some stuff and created a new screenshot without borders that you can use:

image

@ShahzaibIbrahim ShahzaibIbrahim force-pushed the master-94 branch 2 times, most recently from ca6e7fc to 23bfed5 Compare July 24, 2024 13:47
Copy link
Contributor

@HeikoKlare HeikoKlare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a functional perspective, the changes look sound to me. Still the change seem to introduce some unnecessary complexity (or I do not understand the necessity for the complexity so far).

@ShahzaibIbrahim ShahzaibIbrahim marked this pull request as draft July 25, 2024 12:33
@ShahzaibIbrahim ShahzaibIbrahim force-pushed the master-94 branch 4 times, most recently from 5a7806c to c378d22 Compare July 25, 2024 13:47
@ShahzaibIbrahim ShahzaibIbrahim marked this pull request as ready for review July 25, 2024 13:47
Copy link
Contributor

@HeikoKlare HeikoKlare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the recent improvements. The PR looks much better now. Changes are easy to comprehend and look sound. I only have one comment remaining w.r.t. documentation, but since it is about a private method that is easy to comprehend by its name and implementation, I propose to simply remove the potentially irritating documentation.

Disabled image do not work when rescaled. Writing a new method copyImageDataForDisabledOrGrayImages and calling it before init image in win32_getHandle
@HeikoKlare HeikoKlare merged commit ba97568 into eclipse-platform:master Jul 26, 2024
14 checks passed
iloveeclipse added a commit to iloveeclipse/eclipse.platform.swt that referenced this pull request Jul 29, 2024
iloveeclipse added a commit to iloveeclipse/eclipse.platform.swt that referenced this pull request Jul 29, 2024
@iloveeclipse
Copy link
Member

Commit ba97568 broke compilation of snippets project on Linux. Please next time exclude platform specific sources in the Snippets project from classpath on the platforms where the source can't compile.

I've created #1372.

@HeikoKlare: I wonder if we can make the org.eclipse.swt.snippets compilable with Tycho (with some extra config?) that would apply OS specific .classpath fixes to .classpath. I'm not a maven expert, but it would work with good old ant for sure :-)

@laeubi
Copy link
Contributor

laeubi commented Jul 29, 2024

I once opened some requests to JDT to support class folders with different settings so one not need copy around classpath settings or other hacks:

Also see this related Tycho issues:

In the end such issues are that seldom that no one really cared to work on that.

@HeikoKlare
Copy link
Contributor

Thank you for the fix, @iloveeclipse! I missed that the snippet uses OS-specific functionality. I guess that it also breaks on MacOS. I will check that as a follow-up.

It would of course be great to have automated checks for this kind of problems in the build / Tycho. But I am not sure whether it is worth the effort (as indicated by Christoph's comment) or if it would not be even better to introduce some (better) way of declaring and statically checking (already in the IDE) what is part of the SWT API. E.g., having warnings for access of classes/methods marked as @noreference would have made the problem obvious in the IDE (by a warning marker) and in the builds (by introducing an additional warning).

@HeikoKlare
Copy link
Contributor

MacOS is also affected. I've created #1373 to fix that.

@laeubi
Copy link
Contributor

laeubi commented Jul 29, 2024

E.g., having warnings for access of classes/methods marked as @noreference would have made the problem obvious in the IDE (by a warning marker) and in the builds (by introducing an additional warning).

I think this would usually happen if we enable API analysis, but here again, the question is how much effort one want to invest compared to how often are snippets added.

I usually simply have an own project in my workspace named "swt-test" that I use to write a snippet then I copy it over (e.g. for a bugreport or for permanent usage), that works "good enough" at least :-)

@HeikoKlare
Copy link
Contributor

Fully agree that it's a matter of ROI and if this was only relevant for snippets, I agree that only low effort would be worth the benefit in the rather seldom case of having changes/additions to snippets.

I have to admit that I just understood @iloveeclipse's comment completely: the snippet project is not part of the Tycho build at all. I was not aware of that. Doing that should be quite easy and thus a quick win (maybe as an intermediate solution until the requests mentioned by @laeubi for JDT/Tycho have been processed). I will have a look at that.

@HeikoKlare
Copy link
Contributor

I have to admit that I just understood @\iloveeclipse's comment completely: the snippet project is not part of the Tycho build at all. I was not aware of that. Doing that should be quite easy and thus a quick win (maybe as an intermediate solution until the requests mentioned by @\laeubi for JDT/Tycho have been processed). I will have a look at that.

Works fine, but requires at least:

@laeubi can you estimate how difficult it would be to implement that in Tycho?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants